Add firewall blocked domains to AI-generated footers#14517
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 43a6f48:
The firewall logs are uploaded as part of Visual change: Example: <details>
<summary>⚠️ Firewall blocked 2 domains</summary>
> The following domains were blocked by the firewall during workflow execution:
- `blocked.example.com`
- `malicious.site`
</details> |
|
@copilot the details element should be in a warning alert (GitHub markdown syntax) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 006ef9b. The details element is now wrapped in a GitHub warning alert using the Example output: > [!WARNING]
> <details>
> <summary>⚠️ Firewall blocked 2 domains</summary>
>
> The following domains were blocked by the firewall during workflow execution:
>
> - `blocked.example.com`
> - `malicious.site`
>
> </details>The warning alert provides:
|
There was a problem hiding this comment.
Pull request overview
Adds firewall-log awareness to AI-generated footers so users can see (in a collapsed warning block) which domains were blocked by the network firewall during workflow execution.
Changes:
- Add
firewall_blocked_domains.cjshelper to parse firewall logs, sanitize domains, and render a GitHub warning-alert<details>section. - Integrate the blocked-domains footer section into both
generate_footer.cjsandmessages_footer.cjs. - Add tests for the new module and a regression test ensuring no section is emitted when no logs are present; add a workflow markdown file for manual validation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/workflows/test-firewall-blocked-domains-footer.md | Adds a manual test workflow for validating the blocked-domains footer output. |
| actions/setup/js/messages_footer.cjs | Appends blocked-domains warning section to message-based footer generation. |
| actions/setup/js/generate_footer.test.cjs | Adds a test asserting the blocked-domains section is omitted when no logs exist. |
| actions/setup/js/generate_footer.cjs | Appends blocked-domains warning section to the standard footer generation path. |
| actions/setup/js/firewall_blocked_domains.test.cjs | Adds unit tests covering parsing, blocked/allowed detection, sanitization, aggregation, and rendering. |
| actions/setup/js/firewall_blocked_domains.cjs | New module implementing log discovery/parsing + rendering of the warning <details> section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if request was blocked | ||
| const isBlocked = isRequestBlocked(entry.decision, entry.status); | ||
| if (isBlocked) { | ||
| const sanitizedDomain = extractAndSanitizeDomain(entry.domain); | ||
| if (sanitizedDomain && sanitizedDomain !== "-") { | ||
| blockedDomainsSet.add(sanitizedDomain); | ||
| } | ||
| } |
There was a problem hiding this comment.
In getBlockedDomains(), Envoy-style pseudo-domains like error:transaction-end-before-headers will currently be treated as real domains (e.g., truncated to error) and could show up in the footer. The existing firewall log summary code filters out placeholder - and any domain starting with error:; this footer extraction should apply the same filtering before sanitizing/adding to the set so the footer only lists actual blocked domains.
| function parseFirewallLogLine(line) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed || trimmed.startsWith("#")) { | ||
| return null; | ||
| } | ||
|
|
||
| // Split by whitespace but preserve quoted strings | ||
| const fields = trimmed.match(/(?:[^\s"]+|"[^"]*")+/g); | ||
| if (!fields || fields.length < 10) { | ||
| return null; | ||
| } | ||
|
|
||
| // Only validate timestamp (essential for log format detection) | ||
| const timestamp = fields[0]; | ||
| if (!/^\d+(\.\d+)?$/.test(timestamp)) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| timestamp, | ||
| clientIpPort: fields[1], | ||
| domain: fields[2], | ||
| destIpPort: fields[3], | ||
| proto: fields[4], | ||
| method: fields[5], | ||
| status: fields[6], | ||
| decision: fields[7], | ||
| url: fields[8], | ||
| userAgent: fields[9]?.replace(/^"|"$/g, "") || "-", | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Determines if a request was blocked based on decision and status | ||
| * @param {string} decision - Decision field (e.g., TCP_TUNNEL:HIER_DIRECT, NONE_NONE:HIER_NONE) | ||
| * @param {string} status - Status code (e.g., 200, 403, 0) | ||
| * @returns {boolean} True if request was blocked | ||
| */ | ||
| function isRequestBlocked(decision, status) { | ||
| // Check status code first | ||
| const statusCode = parseInt(status, 10); | ||
| if (statusCode === 403 || statusCode === 407) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check decision field | ||
| if (decision.includes("NONE_NONE") || decision.includes("TCP_DENIED")) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for allowed indicators | ||
| if (statusCode === 200 || statusCode === 206 || statusCode === 304) { | ||
| return false; | ||
| } | ||
|
|
||
| if (decision.includes("TCP_TUNNEL") || decision.includes("TCP_HIT") || decision.includes("TCP_MISS")) { | ||
| return false; | ||
| } | ||
|
|
||
| // Default to blocked for safety | ||
| return true; |
There was a problem hiding this comment.
This module duplicates the firewall log parsing and allow/deny classification logic that already exists in actions/setup/js/parse_firewall_logs.cjs (parseFirewallLogLine + decision/status checks). To avoid the two implementations diverging over time, consider extracting shared helpers (e.g., a small firewall_log_core.cjs) or importing/reusing the existing functions where possible.
| it("should not include blocked domains section when no firewall logs exist", () => { | ||
| const result = generateFooter("Test Workflow", "https://github.com/test/repo/actions/runs/123", "", "", undefined, undefined, undefined); | ||
|
|
||
| expect(result).not.toContain("⚠️ Firewall blocked"); | ||
| expect(result).not.toContain("<details>"); | ||
| }); |
There was a problem hiding this comment.
This test assumes /tmp/gh-aw/sandbox/firewall/logs/ does not exist and contains no .log files. Since other tests in this repo create /tmp/gh-aw, this assertion can become environment-dependent/flaky. Make the test deterministic by explicitly removing/isolating the firewall logs path in setup/teardown or by mocking getBlockedDomains() to return an empty list for this case.
| testDir = path.join(os.tmpdir(), `gh-aw-test-firewall-${Date.now()}`); | ||
| fs.mkdirSync(testDir, { recursive: true }); |
There was a problem hiding this comment.
The temporary directory name uses only Date.now(), which is not guaranteed to be unique when Vitest runs test files in parallel. Use a truly unique temp dir (e.g., fs.mkdtempSync() or include pid/random) to avoid rare collisions between concurrent test workers.
| testDir = path.join(os.tmpdir(), `gh-aw-test-firewall-${Date.now()}`); | |
| fs.mkdirSync(testDir, { recursive: true }); | |
| testDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-test-firewall-")); |
Add Firewall Blocked Domains to AI-Generated Footers ✅
Overview
When creating AI-generated footers on items (issues, PRs, comments), the footer now reviews firewall logs and adds a collapsed details message if any domains were blocked. Inside the details section, it shows a list of sanitized domains to inform users about blocked domain access attempts.
The blocked domains section is wrapped in a GitHub warning alert for enhanced visibility and prominence.
Recent Updates (addressing feedback)
Changes made:
> [!WARNING])The firewall logs are uploaded as part of
agent-artifactsand downloaded to/tmp/gh-aw/sandbox/firewall/logs/in the safe-outputs job, which is the same default path used by the module. The implementation works correctly in both contexts:Example Output
When rendered on GitHub, this appears as a yellow/orange warning box with:
Implementation Status: COMPLETE ✅
All checklist items completed and feedback addressed:
generate_footer.cjsandmessages_footer.cjs)Files Changed
New Files:
actions/setup/js/firewall_blocked_domains.cjs(201 lines)actions/setup/js/firewall_blocked_domains.test.cjs(367 lines)pkg/cli/workflows/test-firewall-blocked-domains-footer.md(61 lines)Modified Files:
actions/setup/js/generate_footer.cjsactions/setup/js/generate_footer.test.cjsactions/setup/js/messages_footer.cjsTest Results
JavaScript Tests: ✅
Code Quality: ✅
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.